Thanks for working on the actions.

We have some large PRs and branches pending and need to get to a point where we can freeze PRs, merge all the big kahunas, and only then can we do a massive formatting sweep. We've been waiting for months to do this but there was way too much changing to break history with a bit format PR.

I believe we settled on base black config but with line length of 100 or 120.

@psychedelicious No worries, I totally get that. I personally find it harder to expand on existing files right now because I have to turn off my auto formaters, but that’s me. Just say the word when you want to pull the trigger. Git makes it look harder than it really is to make this branch mergable. Basically it’s as easy as copying all the files from main and running black on them. No need to actually work out merge conflicts.

I hear you, I disabled my autoformat on save too 😮‍💨 the concern was more that with a big formatting commit, we create a boundary that makes diffs hard to reason about, and the code was in very high flux. Getting close now!

Is there a reason you want to do 100-120 character lines? I always liked the standard 88 (leaving an 8 character buffer from the classic 80).

Our monitors are generally bigger than punchcards or hardware terminals now (excellent talk btw), though of course it's just a preference.

The frontend code will be getting the 100 or 120 width treatment soon too

Yeah, I totally get the issue with one big commit that reformats the whole code base and creates a diff wall. If you want, I’ll happily break it up into individual files if that helps. Especially if we can identify files that are not being changed right now. I just didn’t want to spam you with PR’s without first discussing.

I believe we are pretty close to enabling the formatting but I'll request @ebr and @brandonrising for their opinions

Yeah, I totally get the issue with one big commit that reformats the whole code base and creates a diff wall. If you want, I’ll happily break it up into individual files if that helps. Especially if we can identify files that are not being changed right now. I just didn’t want to spam you with PR’s without first discussing.

I actually did the "reformat everything with black" thing last winter during the 2.3 release and one of the gotchas was that "git blame" ended up blaming me for every line of code in the repository. After some searching I found a technique that preserves blame across the black boundary, applied it, and it improved the situation. Do you know about it? If not, I'll try to retrace my steps.

Is there a reason you want to do 100-120 character lines? I always liked the standard 88 (leaving an 8 character buffer from the classic 80).

I prefer the longer lines. 100 is pretty comfortable for me.

@lstein there’s something here about how to get around the git blame issue:
https://black.readthedocs.io/en/stable/guides/introducing_black_to_your_project.html

I’m not entirely clear on how it works based on this link alone, but maybe one big squash merge and then add the commit hash to ‘.git-blame-ignore-revs’?
Does that look like what you did?

@lstein , I think you were looking for this: #2855

(note that if you do squash merges, instead of merge commits, you'll need to do a new PR to add the squashed commit of this PR)

My 2 cents:

• I agree that using and enforcing a formatter/linter is great practice, in general but especially so for OSS development accepting external contributors. It is very cumbersome having to deal with each project's individual coding style. This should be taken care of by computers, not busy humans – I couldn't care less whether this repo is using 80, 88 or 120 char lines. A nicely formatted code base feels way more welcoming to contributions. I'm currently refraining from non-trivial contributions to the backend, even though I would like to, because of the current situation. I cannot be bothered to disable code formatters in 2023, they're integral to my workflow. Big 👍 to the intent behind this PR.
• I wasn't aware of the blame-skip trick. This is awesome, but if it's too impractical still, note that an alternative to the massive whole-repo reformat is to use a formatter that is able to only apply formatting to git-modified lines. This way, historical code will slowly converge to being formatted correctly as actual logic changes are contributed. Sadly PSF black does not yet support this.

@zopieux - Actually by running black with the pre-commit hooks only, black would only be applied to the files changed in the given commit.
However, I personally think that doing it a little bit at a time would make things even less transpararent. Consider a PR that makes a 1 line change to a 500 line file, except black changes all 500 lines. That gets confusing quickly. And with blame you can always go back in time more than one change, so if you see blame pointing to a commit called "apply black", you just hit the button one more time.

I'm calling this out on discord to hopefully grab everyone's attention (and any strongly-dissenting opinions).

This will also close #2522.

black is opinionated (for better or worse), so the line length is the most significant configuration option.

Any in-flight PRs would have to apply black --line-length 120 .

There are also a couple of other options, e.g. --skip-magic-trailing-comma ("Don't use trailing commas as a reason to split lines"), but I'm not sure it's worth our time tweaking that right now.

Re: conflict resolution - @lillekemiker just force-pushed the latest changes, so I will make sure to review it ASAP so we can merge it before any more conflicts are introduced. I will add a commit with the blame-skip trick.

@lillekemiker could I ask you to please rebase on current state of main, apply the 120 char line length change, and force-push one more time? I will make sure this is reviewed today and will try to merge ASAP.

Done!

@lillekemiker: did you use the black --fast option to get baseinvocation.py, commands.py and compel.py to reformat? without --fast, it fails to parse the AST. And/or, which version of black do you have installed?
we also need it added to dev dependencies (I can do that)

Nevermind - not an issue with the latest black

@lillekemiker did you have to pass --include installer/lib/* ? these files are currently being ignored by black due to lib/ present in .gitignore. Seems like we should be able to remove that without consequence, but i'm wondering how you got Black to format these files in the first place.

@lillekemiker did you have to pass --include installer/lib/* ? these files are currently being ignored by black due to lib/ present in .gitignore. Seems like we should be able to remove that without consequence, but i'm wondering how you got Black to format these files in the first place.

I found that if I run black using pre-commit run -a, then it formats everything. I'm guessing either git or pre-commit does the file fetching instead of black when done that way.

. Suggest we merge this as a rebase rather than a squash or merge commit, such that the SHA of the black commit remains unchanged.
.

Both a rebase and a squash will change the SHA. Only a merge commit, or another PR with the SHA of the squash or rebased commit.

. Suggest we merge this as a rebase rather than a squash or merge commit, such that the SHA of the black commit remains unchanged.
.

Both a rebase and a squash will change the SHA. Only a merge commit, or another PR with the SHA of the squash or rebased commit.

right, good point. Let's try a merge commit, though it means the blacked commit won't be on main. Worst case, we'll add another commit with the correct SHA

Let's go! Can finally hack on that codebase :-)